All commit authors signed the Contributor License Agreement.

@pablogsal can you take a look this week? 🙏

@pablogsal can you take a look this week? 🙏

I can try but I have some other PRs first in my review queue :(

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Unfortunately the more I think about it the less I like it: this model is harder to reason about than the previous linked-list model, and I think that matters because it creates a very easy footgun: the current chunk looks like the current stack backing store, but it is not.

Existing live frames may still be in older chunks, while newer frames are in the newest chunk at a matching logical offset. So any code that treats stack_chunk_list as “the stack” instead of “the head of a chain that may contain live frames” can silently become wrong. That already seems to happen here: copying only the head chunk misses older live frames and turns what used to be a cheap bulk-copy unwind into many per-frame remote reads.

The risk is not just external profilers. This makes future runtime/debugger code more fragile because pointer validity and frame ownership now depend on searching the whole chunk chain, not checking the current chunk.

@pablogsal
I don't understand why this PR would cause 1000 remote reads. A 1000 frames stack will only span a few chunks, since they grow exponentially. For very large stacks, there will be fewer chunks to copy, not more.

Existing live frames may still be in older chunks, while newer frames are in the newest chunk at a matching logical offset.

There seems to be a misunderstanding here. No two frames will have the same offset.

This makes future runtime/debugger code more fragile because pointer validity and frame ownership now depend on searching the whole chunk chain, not checking the current chunk.

The stack is a linked list of frames, not chunks. Some of which (generator and coroutine frames) aren't in chunks at all, so tools already need to handle pointers outside of the current chunk.

Overall, I don't see how this really changes anything for an out-of-process profiler: Copy all the chunks, then traverse the stack.

Also, note that the lower, unused part of the current chunk in a stack with multiple chunks will be untouched, so a profiler should be able to detect when it needs to cross to another chunk.

Other profilers like austin currently hard-codes the old _PyStackChunk layout (previous, size, top, data), while this patch changes it to (size, previous, data), so those tools need explicit updates.

That is trade off that tools and libraries make: either they use stable API/ABIs, or they probe CPython internals. If they do the latter, they will need updating every release.

@P403n1x87 would this be a problem for you?

I don't understand why this PR would cause 1000 remote reads. A 1000 frames stack will only span a few chunks, since they grow exponentially. For very large stacks, there will be fewer chunks to copy, not more.

The issue I was pointing at is the current _remote_debugging implementation in this PR: copy_stack_chunks() now processes only the head chunk and sets out_chunks->count = 1. Once frame->previous points into an older chunk, find_frame_in_chunks() misses and we fall back to parse_frame_object(), which reads the frame remotely. That is where the ~966 remote reads for a 1000-frame stack came from.

The fix here is to either restore eager copying of the full stack_chunk_list -> previous chain using the new _PyStackChunk layout, or lazily copy previous chunks when the frame walk crosses out of the copied head chunk.

There seems to be a misunderstanding here. No two frames will have the same offset.

Also, you’re right that “same logical offset” was poor wording. I meant that the new chunk starts allocating at the old stack depth, leaving the lower part of the new chunk unused; not that two frames have the same offset.

Here is a repro:

import os
import subprocess
import sys
import _remote_debugging

DEPTH = 1000

child_code = f"""
import os, sys, time
sys.setrecursionlimit({DEPTH + 1000})

def f(n):
    if n == 0:
        print("READY", os.getpid(), flush=True)
        time.sleep(10)
        return
    f(n - 1)

f({DEPTH})
"""

p = subprocess.Popen(
    [sys.executable, "-c", child_code],
    stdout=subprocess.PIPE,
    text=True,
)

line = p.stdout.readline().strip()
pid = int(line.split()[1])

try:
    unwinder = _remote_debugging.RemoteUnwinder(
        pid,
        all_threads=True,
        cache_frames=False,
        stats=True,
        debug=True,
    )
    trace = unwinder.get_stack_trace()
    frames = sum(len(t.frame_info) for interp in trace for t in interp.threads)

    print("frames:", frames)
    print("stats:", unwinder.get_stats())
finally:
    p.terminate()
    p.wait()

In main we can see:

❯ ./python repro.py
frames: 1002
stats: {'total_samples': 1, 'frame_cache_hits': 0, 'frame_cache_misses': 0, 'frame_cache_partial_hits': 0, 'frames_read_from_cache': 0, 'frames_read_from_memory': 0, 'memory_reads': 4, 'memory_bytes_read': 1200, 'code_object_cache_hits': 1000, 'code_object_cache_misses': 2, 'stale_cache_invalidations': 0, 'frame_cache_hit_rate': 0.0, 'code_object_cache_hit_rate': 99.8003992015968}

with this PR:

frames: 1002
stats: {'total_samples': 1, 'frame_cache_hits': 0, 'frame_cache_misses': 0, 'frame_cache_partial_hits': 0, 'frames_read_from_cache': 0, 'frames_read_from_memory': 0, 'memory_reads': 966, 'memory_bytes_read': 85768, 'code_object_cache_hits': 1000, 'code_object_cache_misses': 2, 'stale_cache_invalidations': 0, 'frame_cache_hit_rate': 0.0, 'code_object_cache_hit_rate': 99.8003992015968}

Notice memory_reads going from 4 to 966

The fix here is to either restore eager copying of the full stack_chunk_list -> previous chain using the new _PyStackChunk layout, or lazily copy previous chunks when the frame walk crosses out of the copied head chunk.

Yes, that's my bad. I made the smallest changes possible to the remote debugging module to get the PR working, but didn't inspect further improvements. Maybe it can be done in a follow-up PR by people more knowledgeable on the module? Or would you consider that a blocker?

Maybe it can be done in a follow-up PR by people more knowledgeable on the module? Or would you consider that a blocker?

This is a blocker. This PR adds a regression and that's not acceptable.

@dpdani I pushed a fix for the concrete _remote_debugging regression: it now walks tstate->stack_chunk_list through previous and copies all stack chunks before traversing frames, instead of only copying the newest chunk. With the fix, the 1000-recursive-frame repro goes from roughly ~966 remote reads / ~85 KB read to 3 reads.

I also added a regression test to make sure deep stacks are resolved from copied chunks rather than falling back to parsing frames individually from remote memory.

That said, I still think this solution is very confusing and too complex. It is harder to reason about where frames live, which chunks are relevant, and what invariants the implementation can rely on. I am worried this makes future changes easier to get subtly wrong.

I investigated an alternative implementation in #149097: instead of replacing the stack with the resizable-array model, it keeps the current chunked- stack invariants and extends the existing one-chunk cache into a small bounded per-thread cache.

I think this is a better direction because it fixes the allocator-thrashing issue without changing where live frames can reside, without changing _PyStackChunk/PyThreadState layout, and without requiring _remote_debugging or external unwinders to learn a new stack model. Active frames remain only in the datastack_chunk -> previous chain; cached chunks are detached and inactive.

The cache is intentionally bounded: currently up to 8 * _PY_DATA_STACK_CHUNK_SIZE, so the memory cost is predictable and much smaller than retaining a high-water-mark stack for the lifetime of the thread.

I cehcked the original repro and it no longer shows per-branch mmap/munmap churn.

To fix the really bad behavior when we are constantly crossing the boundary between chunks, a single cached chunk is sufficient. See #145828

However, calls that cross chunk boundaries will prevent specialization and jitting code as the code keeps having to hit the slow path of not enough space in the current chunk.
Adding more chunks doesn't help, we need bigger chunks so that both caller and callee frame are in the same chunk there is space for the callee in the current chunk.

Also, it allows us to use smaller initial chunks, as don't need to worry about the cost of crossing chunk boundaries.
This doesn't matter much now, but with the increasing prevalence of threading, reducing the memory footprint of individual frames will become more important.

I still think this solution is very confusing and too complex.

I don't think so, I think it is fairly elegant. But it is a bit subtle.
Maybe we just need it better documented (with pictures)?

I'll update https://github.com/python/cpython/blob/main/InternalDocs/frames.md

@pablogsal would it help if the lower, unused part of the chunk were zeroed-out?
That way you have an easy check for when you need to switch chunks, as the bottom of the used part of the current chunk can only grow down.

I don't think so, I think it is fairly elegant. But it is a bit subtle.
Maybe we just need it better documented (with pictures)?

Yes I think with more docs we can make this much more palatable. I want to say that I may be in the minority here so it would be great to get a 2nd opinion from @Fidget-Spinner regarding the complexity argument.

@pablogsal would it help if the lower, unused part of the chunk were zeroed-out?
That way you have an easy check for when you need to switch chunks, as the bottom of the used part of the current chunk can only grow down.

This is how I understand the tradeoff:

The benefit of this approach over the chunk-cache approach is that it addresses a different problem. A cached chunk fixes the allocator-thrashing case, but a hot call near a chunk boundary can still keep failing _PyThreadState_HasStackSpace() and miss specialization/JIT opportunities. Growing the current stack area gives that logical stack depth more headroom, so it can help with the slow-path/JIT issue in a way the cache approach cannot.

The part that still makes me prefer the cache approach is the change in stack/profiler invariants. With the cache approach, inactive chunks are detached, and the active datastack_chunk -> previous chain remains close to the current live stack. With this PR, stack_chunk_list -> previous becomes more of a high-water backing-store chain: fewer chunks, but potentially larger chunks, and older chunks can remain linked after the stack has unwound. That is workable, but it is more subtle and can increase what profilers nee to copy.

On zeroing the lower unused part: I think it could help as a defensive/debugging aid, but I would not rely on it as the correctness mechanism. A profiler/unwinder should still follow _PyInterpreterFrame.previous and then locate that address in the copied stack_chunk_list -> previous chain. A zeroed lower prefix can tell us “there is no frame here”, but it does not identify the previous chunk or replace following the real frame pointer.

It may still be worth doing if we want the unused prefix to be deterministic and make heuristic scans fail cleanly. The main caveat is cost: explicit zeroing can touch pages that would otherwise remain lazily zeroed by the OS, so I would want to avoid zeroing large regions unless we measure that it is negligible.

Perhaps we can alleviate the fact that profilers will need to copy "old zeroed chunks" byt keeping a pointer to the start of the "new" chunks? I can try to profile a bit....maybe I am overreacting to this and the difference is negligible. Will investigate

That is trade off that tools and libraries make: either they use stable API/ABIs, or they probe CPython internals. If they do the latter, they will need updating every release.

@P403n1x87 would this be a problem for you?

As any change of this nature, it is indeed a problem, but as you point out, it is something that maintainers of this kind of tools have to put up with anyway. For the case of Austin specifically, the crucial aspect is being able to work out quickly all the memory chunks with frame data that needs to be copied eagerly to ensure as much data coherence as possible (Austin does not pause threads when unwinding the Python stack). Once that data has been moved over we can work out what to do with it.